Skip to content

BRANCHES.md: Add explanation of branches and using forks #17

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 7,806 commits into from

Conversation

kewillford
Copy link
Member

Adding a file to explain the branches in the repo and how we are using them as well as other helpful hints with forking.

Copy link

@derrickstolee derrickstolee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks good. I wonder why we haven't renamed the gvfs branch. If there is a good reason, then feel free to ignore.

dscho and others added 28 commits December 9, 2018 11:08
In particular on Windows, where the default maximum path length is quite
small, but there are ways to circumvent that limit in many cases, it is
very important that users be given an indication why their command
failed because of too long paths when it did.

This test case makes sure that a warning is issued that would have
helped the user who reported Git for Windows' issue 521:

	git-for-windows#521

Signed-off-by: Johannes Schindelin <[email protected]>
The CreateProcessW() function does not really support spaces in its
first argument, lpApplicationName. But it supports passing NULL as
lpApplicationName, which makes it figure out the application from the
(possibly quoted) first argument of lpCommandLine.

Let's use that trick (if we are certain that the first argument matches
the executable's path) to support launching programs whose path contains
spaces.

This fixes https://github.com/git-for-windows/git/issue/692

Signed-off-by: Johannes Schindelin <[email protected]>
Signed-off-by: Jeff Hostetler <[email protected]>
Signed-off-by: Johannes Schindelin <[email protected]>
Teach FSCACHE to remember "not found" directories.

This is a performance optimization.

FSCACHE is a performance optimization available for Windows.  It
intercepts Posix-style lstat() calls into an in-memory directory
using FindFirst/FindNext.  It improves performance on Windows by
catching the first lstat() call in a directory, using FindFirst/
FindNext to read the list of files (and attribute data) for the
entire directory into the cache, and short-cut subsequent lstat()
calls in the same directory.  This gives a major performance
boost on Windows.

However, it does not remember "not found" directories.  When STATUS
runs and there are missing directories, the lstat() interception
fails to find the parent directory and simply return ENOENT for the
file -- it does not remember that the FindFirst on the directory
failed. Thus subsequent lstat() calls in the same directory, each
re-attempt the FindFirst.  This completely defeats any performance
gains.

This can be seen by doing a sparse-checkout on a large repo and
then doing a read-tree to reset the skip-worktree bits and then
running status.

This change reduced status times for my very large repo by 60%.

Signed-off-by: Jeff Hostetler <[email protected]>
Signed-off-by: Johannes Schindelin <[email protected]>
A ton of Git commands simply do not read (or at least parse) the core.*
settings. This is not good, as Git for Windows relies on the
core.longPaths setting to be read quite early on.

So let's just make sure that all commands read the config and give
platform_core_config() a chance.

This patch teaches tons of Git commands to respect the config setting
`core.longPaths = true`, including `pack-refs`, thereby fixing
git-for-windows#1218

Signed-off-by: Johannes Schindelin <[email protected]>
Teach read_directory_recursive() and add_excludes() to
be aware of optional fscache and avoid trying to open()
and fstat() non-existant ".gitignore" files in every
directory in the worktree.

The current code in add_excludes() calls open() and then
fstat() for a ".gitignore" file in each directory present
in the worktree.  Change that when fscache is enabled to
call lstat() first and if present, call open().

This seems backwards because both lstat needs to do more
work than fstat.  But when fscache is enabled, fscache will
already know if the .gitignore file exists and can completely
avoid the IO calls.  This works because of the lstat diversion
to mingw_lstat when fscache is enabled.

This reduced status times on a 350K file enlistment of the
Windows repo on a NVMe SSD by 0.25 seconds.

Signed-off-by: Jeff Hostetler <[email protected]>
Teach "add" to use preload-index and fscache features
to improve performance on very large repositories.

During an "add", a call is made to run_diff_files()
which calls check_remove() for each index-entry.  This
calls lstat().  On Windows, the fscache code intercepts
the lstat() calls and builds a private cache using the
FindFirst/FindNext routines, which are much faster.

Somewhat independent of this, is the preload-index code
which distributes some of the start-up costs across
multiple threads.

We need to keep the call to read_cache() before parsing the
pathspecs (and hence cannot use the pathspecs to limit any preload)
because parse_pathspec() is using the index to determine whether a
pathspec is, in fact, in a submodule. If we would not read the index
first, parse_pathspec() would not error out on a path that is inside
a submodule, and t7400-submodule-basic.sh would fail with

	not ok 47 - do not add files from a submodule

We still want the nice preload performance boost, though, so we simply
call read_cache_preload(&pathspecs) after parsing the pathspecs.

Signed-off-by: Jeff Hostetler <[email protected]>
Signed-off-by: Johannes Schindelin <[email protected]>
Make fscache_enabled() function public rather than static.
Remove unneeded fscache_is_enabled() function.
Change is_fscache_enabled() macro to call fscache_enabled().

is_fscache_enabled() now takes a pathname so that the answer
is more precise and mean "is fscache enabled for this pathname",
since fscache only stores repo-relative paths and not absolute
paths, we can avoid attempting lookups for absolute paths.

Signed-off-by: Jeff Hostetler <[email protected]>
Fix regression described in:
git-for-windows#1392

which was introduced in:
git-for-windows@b235337

Problem Symptoms
================
When the user has a .gitignore file that is a symlink, the fscache
optimization introduced above caused the stat-data from the symlink,
rather that of the target file, to be returned.  Later when the ignore
file was read, the buffer length did not match the stat.st_size field
and we called die("cannot use <path> as an exclude file")

Optimization Rationale
======================
The above optimization calls lstat() before open() primarily to ask
fscache if the file exists.  It gets the current stat-data as a side
effect essentially for free (since we already have it in memory).
If the file does not exist, it does not need to call open().  And
since very few directories have .gitignore files, we can greatly
reduce time spent in the filesystem.

Discussion of Fix
=================
The above optimization calls lstat() rather than stat() because the
fscache only intercepts lstat() calls.  Calls to stat() stay directed
to the mingw_stat() completly bypassing fscache.  Furthermore, calls
to mingw_stat() always call {open, fstat, close} so that symlinks are
properly dereferenced, which adds *additional* open/close calls on top
of what the original code in dir.c is doing.

Since the problem only manifests for symlinks, we add code to overwrite
the stat-data when the path is a symlink.  This preserves the effect of
the performance gains provided by the fscache in the normal case.

Signed-off-by: Jeff Hostetler <[email protected]>
When I do git fetch, git call file stats under .git/objects for each
refs. This takes time when there are many refs.

By enabling fscache, git takes file stats by directory traversing and that
improved the speed of fetch-pack for repository having large number of
refs.

In my windows workstation, this improves the time of `git fetch` for
chromium repository like below. I took stats 3 times.

* With this patch
TotalSeconds: 9.9825165
TotalSeconds: 9.1862075
TotalSeconds: 10.1956256
Avg: 9.78811653333333

* Without this patch
TotalSeconds: 15.8406702
TotalSeconds: 15.6248053
TotalSeconds: 15.2085938
Avg: 15.5580231

Signed-off-by: Takuto Ikuta <[email protected]>
This is retry of git-for-windows#1419.

I added flush_fscache macro to flush cached stats after disk writing
with tests for regression reported in git-for-windows#1438 and git-for-windows#1442.

git checkout checks each file path in sorted order, so cache flushing does not
make performance worse unless we have large number of modified files in
a directory containing many files.

Using chromium repository, I tested `git checkout .` performance when I
delete 10 files in different directories.
With this patch:
TotalSeconds: 4.307272
TotalSeconds: 4.4863595
TotalSeconds: 4.2975562
Avg: 4.36372923333333

Without this patch:
TotalSeconds: 20.9705431
TotalSeconds: 22.4867685
TotalSeconds: 18.8968292
Avg: 20.7847136

I confirmed this patch passed all tests in t/ with core_fscache=1.

Signed-off-by: Takuto Ikuta <[email protected]>
On file systems that support it, this can dramatically speed up operations
like add, commit, describe, rebase, reset, rm that would otherwise have to
lstat() every file to "re-match" the stat information in the index to that
of the file system.

On a synthetic repo with 1M files, "git reset" dropped from 52.02 seconds to
14.42 seconds for a savings of 72%.

Signed-off-by: Ben Peart <[email protected]>
This branch introduces support for reading the "Windows-wide" Git
configuration from `%PROGRAMDATA%\Git\config`. As these settings are
intended to be shared between *all* Git-related software, that config
file takes an even lower precedence than `$(prefix)/etc/gitconfig`.

Signed-off-by: Johannes Schindelin <[email protected]>
This will come in handy in the next commit.

Signed-off-by: JiSeop Moon <[email protected]>
Signed-off-by: Johannes Schindelin <[email protected]>
It is a known issue that a rename() can fail with an "Access denied"
error at times, when copying followed by deleting the original file
works. Let's just fall back to that behavior.

Signed-off-by: JiSeop Moon <[email protected]>
Signed-off-by: Johannes Schindelin <[email protected]>
In preparation for making this function a bit more complicated (to allow
for special-casing the `ContainerMappedDirectories` in Windows
containers, which look like a symbolic link, but are not), let's move it
out of the header.

Signed-off-by: JiSeop Moon <[email protected]>
Signed-off-by: Johannes Schindelin <[email protected]>
... even if they may look like them.

As looking up the target of the "symbolic link" (just to see whether it
starts with `/ContainerMappedDirectories/`) is pretty expensive, we
do it when we can be *really* sure that there is a possibility that this
might be the case.

Signed-off-by: Johannes Schindelin <[email protected]>
Signed-off-by: JiSeop Moon <[email protected]>
When a third-party tool periodically runs `git status` in order to keep
track of the state of the working tree, it is a bad idea to lock the
index: it might interfere with interactive commands executed by the
user, e.g. when the user wants to commit files.

Git for Windows introduced the `--no-lock-index` option a long time ago
to fix that (it made it into Git for Windows v2.9.2(3)) by simply
avoiding to write that file.

The downside is that the periodic `git status` calls will be a little
bit more wasteful because they may have to refresh the index repeatedly,
only to throw away the updates when it exits. This cannot really be
helped, though, as tools wanting to get a periodic update of the status
have no way to predict when the user may want to lock the index herself.

Sadly, a competing approach was submitted (by somebody who apparently
has less work on their plate than this maintainer) that made it into
v2.15.0 but is *different*: instead of a `git status`-only option, it is
an option that comes *before* the Git command and is called differently,
too.

Let's give previous users a chance to upgrade to newer Git for Windows
versions by handling the `--no-lock-index` option, still, though with a
big fat warning.

Signed-off-by: Johannes Schindelin <[email protected]>
It was a bad idea to just remove that option from Git for Windows
v2.15.0, as early users of that (still experimental) option would have
been puzzled what they are supposed to do now.

So let's reintroduce the flag, but make sure to show the user good
advice how to fix this going forward.

We'll remove this option in a more orderly fashion either in v2.16.0 or
in v2.17.0.

Signed-off-by: Johannes Schindelin <[email protected]>
The option is deprecated now, and we better make sure that keeps saying
so until we finally remove it.

Suggested by Kevin Willford.

Signed-off-by: Johannes Schindelin <[email protected]>
From the documentation of said setting:

	This boolean will enable fsync() when writing object files.

	This is a total waste of time and effort on a filesystem that
	orders data writes properly, but can be useful for filesystems
	that do not use journalling (traditional UNIX filesystems) or
	that only journal metadata and not file contents (OS X’s HFS+,
	or Linux ext3 with "data=writeback").

The most common file system on Windows (NTFS) does not guarantee that
order, therefore a sudden loss of power (or any other event causing an
unclean shutdown) would cause corrupt files (i.e. files filled with
NULs). Therefore we need to change the default.

Note that the documentation makes it sound as if this causes really bad
performance. In reality, writing loose objects is something that is done
only rarely, and only a handful of files at a time.

Signed-off-by: Johannes Schindelin <[email protected]>
The TerminateProcess() function does not actually leave the child
processes any chance to perform any cleanup operations. This is bad
insofar as Git itself expects its signal handlers to run.

A symptom is e.g. a left-behind .lock file that would not be left behind
if the same operation was run, say, on Linux.

To remedy this situation, we use an obscure trick: we inject a thread
into the process that needs to be killed and to let that thread run the
ExitProcess() function with the desired exit status. Thanks J Wyman for
describing this trick.

The advantage is that the ExitProcess() function lets the atexit
handlers run. While this is still different from what Git expects (i.e.
running a signal handler), in practice Git sets up signal handlers and
atexit handlers that call the same code to clean up after itself.

In case that the gentle method to terminate the process failed, we still
fall back to calling TerminateProcess(), but in that case we now also
make sure that processes spawned by the spawned process are terminated;
TerminateProcess() does not give the spawned process a chance to do so
itself.

Please note that this change only affects how Git for Windows tries to
terminate processes spawned by Git's own executables. Third-party
software that *calls* Git and wants to terminate it *still* need to make
sure to imitate this gentle method, otherwise this patch will not have
any effect.

Signed-off-by: Johannes Schindelin <[email protected]>
Previously, we did not install any handler for Ctrl+C, but now we really
want to because the MSYS2 runtime learned the trick to call the
ConsoleCtrlHandler when Ctrl+C was pressed.

With this, hitting Ctrl+C while `git log` is running will only terminate
the Git process, but not the pager. This finally matches the behavior on
Linux and on macOS.

Signed-off-by: Johannes Schindelin <[email protected]>
…sses

When spawning child processes, we really should be careful which file
handles we let them inherit.

This is doubly important on Windows, where we cannot rename, delete, or
modify files if there is still a file handle open.

Sadly, we have to guard this test inside #ifdef WIN32: we need to use
the value of the HANDLE directly, and that concept does not exist on
Linux/Unix.

Signed-off-by: Johannes Schindelin <[email protected]>
For some reason, when being called via TortoiseGit the standard handles,
or at least what is returned by _get_osfhandle(0) for standard input,
can take on the value (HANDLE)-2 (which is not a legal value, according
to the documentation).

Even if this value is not documented anywhere, CreateProcess() seems to
work fine without complaints if hStdInput set to this value.

In contrast, the upcoming code to restrict which file handles get
inherited by spawned processes would result in `ERROR_INVALID_PARAMETER`
when including such handle values in the list.

To help this, special-case the value (HANDLE)-2 returned by
_get_osfhandle() and replace it with INVALID_HANDLE_VALUE, which will
hopefully let the handle inheritance restriction work even when called
from TortoiseGit.

This fixes git-for-windows#1481

Signed-off-by: Johannes Schindelin <[email protected]>
This topic branch conflicts with the next change that will change the
way we call `CreateProcessW()`. So let's merge it early, to avoid merge
conflicts during a merge (because we would have to resolve this with
every single merging-rebase).

Signed-off-by: Johannes Schindelin <[email protected]>
By default, CreateProcess() does not inherit any open file handles,
unless the bInheritHandles parameter is set to TRUE. Which we do need to
set because we need to pass in stdin/stdout/stderr to talk to the child
processes. Sadly, this means that all file handles (unless marked via
O_NOINHERIT) are inherited.

This lead to problems in GVFS Git, where a long-running read-object hook
is used to hydrate missing objects, and depending on the circumstances,
might only be called *after* Git opened a file handle.

Ideally, we would not open files without O_NOINHERIT unless *really*
necessary (i.e. when we want to pass the opened file handle as standard
handle into a child process), but apparently it is all-too-easy to
introduce incorrect open() calls: this happened, and prevented updating
a file after the read-object hook was started because the hook still
held a handle on said file.

Happily, there is a solution: as described in the "Old New Thing"
https://blogs.msdn.microsoft.com/oldnewthing/20111216-00/?p=8873 there
is a way, starting with Windows Vista, that lets us define precisely
which handles should be inherited by the child process.

And since we bumped the minimum Windows version for use with Git for
Windows to Vista with v2.10.1 (i.e. a *long* time ago), we can use this
method. So let's do exactly that.

We need to make sure that the list of handles to inherit does not
contain duplicates; Otherwise CreateProcessW() would fail with
ERROR_INVALID_ARGUMENT.

While at it, stop setting errno to ENOENT unless it really is the
correct value.

Also, fall back to not limiting handle inheritance under certain error
conditions (e.g. on Windows 7, which is a lot stricter in what handles
you can specify to limit to).

Signed-off-by: Johannes Schindelin <[email protected]>
jamill and others added 2 commits December 11, 2018 12:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.